Extract CVCMerkleProof from the VC creation by william-brooks · Pull Request #226 · identity-com/credential-commons

william-brooks · 2023-04-18T10:45:35Z

This update separates the CvcMerkleProof from VC creation allowing for alternative proof mechanisms to be used.

Signed-off-by: William Brooks william@identity.org
Reviewed-by: Julian Spring julianspring@identity.org
Reviewed-by: Tighe Barris 4658684+tbarri@users.noreply.github.com

…r supporting features that rely on CvcMerkleRoot like attestations

JulianSpring · 2023-04-18T11:32:00Z

src/proof/CvcMerkleProof/Ed25519SignerVerifier.ts

@@ -0,0 +1,59 @@
+/* eslint-disable @typescript-eslint/no-explicit-any */


Instead of doing this file wide could we do this next to the any usage :)

I've made the change where it makes sense... there is one file that is completely riddled with anys

src/proof/CvcMerkleProof/Ed25519SignerVerifier.ts

JulianSpring · 2023-04-18T11:35:00Z

src/proof/CvcMerkleProof/Ed25519SignerVerifier.ts

+
+        const vm = await didUtil.findVerificationMethod(did, this._verificationMethod);
+
+        if(!vm) return false;


Should we return an error?

No... it was like that... but, following suite on other libraries, if it is unable to verify (for whatever reason) return false

src/proof/Proof.ts

JulianSpring · 2023-04-18T11:46:18Z

src/vc/VerifiableCredential.ts

+import validUrl from 'valid-url';
+
+function validIdentifiers() {
+    const vi = _.map(definitions, (d: { identifier: any; }) => d.identifier);


I think we should use the typescript array functions instead of lodash?

Lodash is all over... mostly just copy/paste from the existing implementation

I have created a separate ticket for this

src/vc/VerifiableCredential.ts

JulianSpring · 2023-04-18T12:11:51Z

src/vc/VerifiableCredential.ts

                identifier: this.identifier,
                expirationDate: this.expirationDate,
-                version: this.version,
+                // version: this.version,


Do we need this?

I added a TODO with ticket number

JulianSpring · 2023-04-18T12:12:48Z

src/vc/VerifiableCredential.ts

        }
+    }
+
+    isMatch = (constraints: any) => {


I think we should stay consistant with our types of functions? What do you think?

Agreed, mostly because I'd spend another few days fixing pre-existing broken types

Sorry I totally misunderstood you - ignore previous comment.

Agreed, fixing

__integrations__/credentials/VerifiableCredential.test.js

JulianSpring · 2023-04-18T12:20:45Z

src/proof/JsonWebSignatureProof.ts

-        if(!foundKey) {
-            throw new Error(`No Verification Method found for ${iri}`);
-        }
+        const foundKey = doc.verificationMethod?.find((pk: any) => pk.id.startsWith(iri));


should we check if foundKey is undefined ?

This would silently fail (correctly imo), but have updated it to be more explicit

__test__/lib/resolver.test.js

__test__/proof/EcdsaSecp256k1RecoverySignature2020.test.js

__test__/proof/JsonWebSignatureProof.test.js

tbarri · 2023-04-19T07:38:04Z

__integrations__/credentials/VerifiableCredential.test.js

+    const unsignedCred = await VerifiableCredential.create({
+      issuer: didTestUtil.DID_CONTROLLER,
+      identifier: 'credential-cvc:Identity-v3',
+      subject: credentialSubject,
+      claims: [name, dob],
+      expiry: null,
+    });


In general, if we're copying large blocks of boilerplate code over and over, we need to please refactor it into a single location.

__test__/index.test.js

tbarri

This is a fairly large commit, and no doubt valuable work. Thank you for putting the effort into this. Something for the future though:

Try break the PR into even smaller commits, and squash the commits that don't matter. For example, commit Extracted CvcMerkleProof with tests doesn't contain anything meaningful. As such, it shouldn't be in this PR. There's 6 commits all named Extract CVCMerkleProof from the VC creation.

Similarly, if you make large sweeping reformat changes as was done in Updates to support CvcMerkleRoot as separate to a VC, along with others, please keep it in a separate commit. It's almost impossible to separate your logic from the reformat changes.

Ideally, we want the PRs to be presented as you intend to merge them. Working diffs need to please be squashed / merged / dropped before sending out for review.

Please don't stress about this PR series - but let's keep this in mind for the next PR :)

tbarri · 2023-04-19T07:54:08Z

__test__/lib/resolver.test.js

            const signer = createSigner(document, keypair);

-            const credential = await createCredential(document, signer, keyResolver);
+            const cvcMerkleProof = new CvcMerkleProof(signer);
+
+            const credential = await createCredential(document, cvcMerkleProof);

-            const verified = await credential.verifyMerkletreeSignature();
+            const verified = await cvcMerkleProof.verify(credential);

            expect(verified).toBe(true);


I think it would be really valuable if we wrote these unit tests in terms of the interfaces (export default interface Proof<K>). This way, we can drop any concrete implementation into the unit tests, and validate that they work.

This update separates the CvcMerkleProof from VC creation allowing for alternative proof mechanisms to be used. PR: #226 Signed-off-by: William Brooks <william@identity.org> Reviewed-by: Julian Spring <julianspring@identity.org> Reviewed-by: Tighe Barris <4658684+tbarri@users.noreply.github.com>

JulianSpring

Please wait for Tighe too 🙏

William Brooks added 5 commits April 13, 2023 11:27

Fixed broken tests

558d1cf

Updates to support CvcMerkleRoot as separate to a VC, along with othe…

1c9fd6d

…r supporting features that rely on CvcMerkleRoot like attestations

Extracted CvcMerkleProof with tests

051450e

Added husky

2588040

Extracted cvc merkle proof from a VC

4678f36

william-brooks changed the base branch from master to develop April 18, 2023 10:45

william-brooks marked this pull request as ready for review April 18, 2023 11:28

william-brooks requested review from JulianSpring and tbarri April 18, 2023 12:16

JulianSpring reviewed Apr 18, 2023

View reviewed changes